Skip to content

feat(spec): read timestamp_ns/timestamptz_ns default values in the Literal JSON codec#2832

Merged
blackmwk merged 2 commits into
apache:mainfrom
c-thiel:ct/timestamp-ns-default-values-json
Jul 16, 2026
Merged

feat(spec): read timestamp_ns/timestamptz_ns default values in the Literal JSON codec#2832
blackmwk merged 2 commits into
apache:mainfrom
c-thiel:ct/timestamp-ns-default-values-json

Conversation

@c-thiel

@c-thiel c-thiel commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Which issue does this PR close?

We don't have an issue for this. Deserializing Timestamp NS default values failed.

What changes are included in this PR?

try_into_json serialized TimestampNs/TimestamptzNs but try_from_json had no matching arms, so a nanosecond-timestamp single value (e.g. a column default) failed to deserialize (fell to the DataInvalid catch-all). Add the read arms
mirroring the microsecond ones, plus datetime_to_nanoseconds / datetimetz_to_nanoseconds helpers. Also adds json_timestamp_ns / json_timestamptz_ns tests.

Are these changes tested?

Yes

@Kurtiscwright Kurtiscwright left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this looks great.

QQ: Should we add tests with different time zones?

Mirror Java's testInvalidTimestamptz{,Nano}: only +00:00 is valid per
spec, so a non-UTC offset must be rejected.
@c-thiel

c-thiel commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator Author

Thank you, this looks great.

QQ: Should we add tests with different time zones?

Good call, added tests for it. Spec says the offset must be +00:00 and Java enforces the same (isUTCTimestamptz), so I added json_timestamptz_ns_rejects_non_utc_offset + the micros counterpart asserting a non-UTC offset is rejected (matching Java's testInvalidTimestamptz{,Nano}). +00:00 round-trip is already covered.

One note: we accept only the literal +00:00, not Z like Java does — spec mandates +00:00 and both writers emit it, so round-trips are fine. I would keep it strict for now.

@blackmwk blackmwk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @c-thiel for this fix!

@blackmwk
blackmwk merged commit 743d098 into apache:main Jul 16, 2026
21 checks passed
NoahKusaba added a commit to NoahKusaba/iceberg-rust that referenced this pull request Jul 16, 2026
…ion-ballista

Brings the ballista branch in-line with catalog-config: Variant support
(apache#2188), encrypted parquet data-file reads (apache#2584), timestamp_ns default
literals (apache#2832), and the format_version auto-selection in
SchemaProvider::create_table. Regenerated Cargo.lock and all
DEPENDENCIES.rust.tsv files; workspace + tests compile on datafusion 54 /
ballista 54.
NoahKusaba pushed a commit to NoahKusaba/iceberg-rust that referenced this pull request Jul 16, 2026
…teral JSON codec (apache#2832)

## Which issue does this PR close?
We don't have an issue for this. Deserializing Timestamp NS default
values failed.

## What changes are included in this PR?

`try_into_json` serialized TimestampNs/TimestamptzNs but `try_from_json`
had no matching arms, so a nanosecond-timestamp single value (e.g. a
column default) failed to deserialize (fell to the DataInvalid
catch-all). Add the read arms
mirroring the microsecond ones, plus `datetime_to_nanoseconds` /
`datetimetz_to_nanoseconds` helpers. Also adds json_timestamp_ns /
json_timestamptz_ns tests.

## Are these changes tested?

Yes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants